pgsql-hackers
❮
Oversight in slab.c SlabContextCreate(), initial memory allocation size is not populated to context->mem_allocated
- Jump to comment-1reid.thompson@crunchydata.com2022-07-29T16:43:45+00:00Hi, Both aset.c and generation.c populate mem_allocated in AllocSetContextCreateInternal(), GenerationContextCreate() respectively. aset.c /* Finally, do the type-independent part of context creation */ MemoryContextCreate((MemoryContext) set, T_AllocSetContext, &AllocSetMethods, parent, name); ((MemoryContext) set)->mem_allocated = firstBlockSize; return (MemoryContext) set; } generation.c /* Finally, do the type-independent part of context creation */ MemoryContextCreate((MemoryContext) set, T_GenerationContext, &GenerationMethods, parent, name); ((MemoryContext) set)->mem_allocated = firstBlockSize; return (MemoryContext) set; } slab.c does not in SlabContextCreate(). Is this intentional, it seems to be an oversight to me. /* Finally, do the type-independent part of context creation */ MemoryContextCreate((MemoryContext) slab, T_SlabContext, &SlabMethods, parent, name); return (MemoryContext) slab; } -- Reid Thompson Senior Software Engineer Crunchy Data, Inc. reid.thompson@crunchydata.com www.crunchydata.com
- Jump to comment-1nathandbossart@gmail.com2022-07-29T17:48:40+00:00On Fri, Jul 29, 2022 at 12:43:45PM -0400, Reid Thompson wrote: > slab.c > does not in SlabContextCreate(). Is this intentional, it seems to be an > oversight to me. > > /* Finally, do the type-independent part of context creation */ > MemoryContextCreate((MemoryContext) slab, > T_SlabContext, > &SlabMethods, > parent, > name); > > return (MemoryContext) slab; > } IIUC this is because the header is tracked separately from the first regular block, unlike aset.c. See the following comment: /* * Allocate the context header. Unlike aset.c, we never try to combine * this with the first regular block; not worth the extra complication. */ You'll also notice that the "reset" and "free" functions in aset.c and generation.c have special logic for "keeper" blocks. Here is a relevant comment from AllocSetReset(): * Actually, this routine has some discretion about what to do. * It should mark all allocated chunks freed, but it need not necessarily * give back all the resources the set owns. Our actual implementation is * that we give back all but the "keeper" block (which we must keep, since * it shares a malloc chunk with the context header). In this way, we don't * thrash malloc() when a context is repeatedly reset after small allocations, * which is typical behavior for per-tuple contexts. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
- Jump to comment-1tgl@sss.pgh.pa.us2022-07-29T17:55:10+00:00Nathan Bossart <nathandbossart@gmail.com> writes: > On Fri, Jul 29, 2022 at 12:43:45PM -0400, Reid Thompson wrote: >> slab.c >> does not in SlabContextCreate(). Is this intentional, it seems to be an >> oversight to me. > IIUC this is because the header is tracked separately from the first > regular block, unlike aset.c. That doesn't make it not an oversight, though. It looks like aset.c thinks that mem_allocated includes all the context's overhead, whereas this implementation doesn't seem to have that result. The comments associated with mem_allocated are sufficiently vague that it's impossible to tell which implementation is correct. Maybe we don't really care, but ... regards, tom lane
- Jump to comment-1nathandbossart@gmail.com2022-07-29T18:23:47+00:00On Fri, Jul 29, 2022 at 01:55:10PM -0400, Tom Lane wrote: > Nathan Bossart <nathandbossart@gmail.com> writes: >> On Fri, Jul 29, 2022 at 12:43:45PM -0400, Reid Thompson wrote: >>> slab.c >>> does not in SlabContextCreate(). Is this intentional, it seems to be an >>> oversight to me. > >> IIUC this is because the header is tracked separately from the first >> regular block, unlike aset.c. > > That doesn't make it not an oversight, though. It looks like aset.c > thinks that mem_allocated includes all the context's overhead, whereas > this implementation doesn't seem to have that result. The comments > associated with mem_allocated are sufficiently vague that it's impossible > to tell which implementation is correct. Maybe we don't really care, > but ... Hm. mmgr/README indicates the following note about mem_allocated: * inquire about the total amount of memory allocated to the context (the raw memory from which the context allocates chunks; not the chunks themselves) AFAICT MemoryContextMemAllocated() is only used for determining when to spill to disk for hash aggegations at the moment. I don't know whether I'd classify this as an oversight or if it even makes any meaningful difference, but consistency among the different implementations is probably desirable either way. So, I guess I'm +1 for including the memory context header in mem_allocated in this case. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
- Jump to comment-1tomas.vondra@enterprisedb.com2022-07-29T19:16:51+00:00On 7/29/22 20:23, Nathan Bossart wrote: > On Fri, Jul 29, 2022 at 01:55:10PM -0400, Tom Lane wrote: >> Nathan Bossart <nathandbossart@gmail.com> writes: >>> On Fri, Jul 29, 2022 at 12:43:45PM -0400, Reid Thompson wrote: >>>> slab.c >>>> does not in SlabContextCreate(). Is this intentional, it seems to be an >>>> oversight to me. >> >>> IIUC this is because the header is tracked separately from the first >>> regular block, unlike aset.c. >> >> That doesn't make it not an oversight, though. It looks like aset.c >> thinks that mem_allocated includes all the context's overhead, whereas >> this implementation doesn't seem to have that result. The comments >> associated with mem_allocated are sufficiently vague that it's impossible >> to tell which implementation is correct. Maybe we don't really care, >> but ... > > Hm. mmgr/README indicates the following note about mem_allocated: > > * inquire about the total amount of memory allocated to the context > (the raw memory from which the context allocates chunks; not the > chunks themselves) > > AFAICT MemoryContextMemAllocated() is only used for determining when to > spill to disk for hash aggegations at the moment. I don't know whether I'd > classify this as an oversight or if it even makes any meaningful > difference, but consistency among the different implementations is probably > desirable either way. So, I guess I'm +1 for including the memory context > header in mem_allocated in this case. > I don't think this can make meaningful difference - as you mention, we only really use this to decide when to spill to disk etc. So maybe you'll spill a bit sooner, but the work_mem is pretty crude threshold anyway, people don't tune it to an exact byte value (which would be pretty futile anyway). OTOH it does seem like an oversight, or at least an inconsistency with the two other contexts, so if anyone feels like tweaking it ... regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
- Jump to comment-1horikyota.ntt@gmail.com2022-08-01T07:57:44+00:00At Fri, 29 Jul 2022 21:16:51 +0200, Tomas Vondra <tomas.vondra@enterprisedb.com> wrote in > I don't think this can make meaningful difference - as you mention, we > only really use this to decide when to spill to disk etc. So maybe > you'll spill a bit sooner, but the work_mem is pretty crude threshold > anyway, people don't tune it to an exact byte value (which would be > pretty futile anyway). From another perspective.. SlabStats includes the header size into its total size. So it reports a different total size from MemoryContextMemAllocated() (For example, 594 bytes vs 0). Since this is an inconsistency within slab.c, no users will notice that difference in the field. > OTOH it does seem like an oversight, or at least an inconsistency with > the two other contexts, so if anyone feels like tweaking it ... regards. -- Kyotaro Horiguchi NTT Open Source Software Center